Additional annotation parsing #919

reicheratwork · 2021-08-30T09:40:32Z

Signed-off-by: Martijn Reicher martijn.reicher@adlinktech.com

k0ekk0ek · 2021-08-30T09:43:43Z

Thanks @reicheratwork! Better to have a member in the corresponding nodes to indicate if something's optional so that each backend can use it. Also, would be nice to verify @optional can only be assigned to the correct types (probably structs and unions only?).

reicheratwork · 2021-08-30T10:00:23Z

Thanks @reicheratwork! Better to have a member in the corresponding nodes to indicate if something's optional so that each backend can use it. Also, would be nice to verify @optional can only be assigned to the correct types (probably structs and unions only?).

Can't @optional not be assigned to basic types as well?
If i recall correctly, it is an annotation to members, right?
So having a flag or something would then be on the idl_member_t as the most logical location, IMO.

reicheratwork · 2021-08-30T14:49:25Z

@k0ekk0ek in my most recent push, I added an idl_boolean_t is_optional to the idl_member_t class and created a utility function checking for this, so that the different backend implementations will not have to create their own

dpotman

I've reviewed these changes and found a few issues that need a fix, mainly the incomplete @default implementation and using the is_optional field instead of iterating through the annotations in idl_is_optional. And some nit-picking on the consistency of the coding style.

dpotman · 2021-09-08T15:19:41Z

src/idl/src/annotation.c

+  idl_node_t *node)
+{
+  if (!idl_is_member(node)
+   && !idl_is_case(node)) {


In DDS XTypes spec section 7.2.2.4.4.4.7: "Union members, including the discriminator, shall never be optional", so I think that @optional should only be allowed on nodes of type idl_member_t

This is corrected in the new iteration.

dpotman · 2021-09-08T15:21:25Z

src/idl/src/annotation.c

+  if (annotation_appl->parameters
+   && annotation_appl->parameters->const_expr
+   && idl_mask(annotation_appl->parameters->const_expr) != (IDL_LITERAL | IDL_BOOL)) {
+        idl_error(pstate, idl_location(annotation_appl),


Nit-pick: I'd prefer to have the indenting consistent with the rest of this file

dpotman · 2021-09-08T15:24:22Z

src/idl/include/idl/tree.h

@@ -299,6 +299,7 @@ struct idl_member {
  idl_declarator_t *declarators;
  /* metadata */
  idl_boolean_t key;
+  idl_boolean_t is_optional;


Nit-pick: is_key and is_optional or key and optional

You are correct, in the newest iteration this is fixed.
However, some member names cannot be chosen as they are reserved keywords (such as default), so there I may need to use little workarounds.

dpotman · 2021-09-09T08:12:50Z

src/idl/src/annotation.c

+          "@optional cannot have non-boolean values assigned");
+    return IDL_RETCODE_SEMANTIC_ERROR;
+  }
+


Shouldn't ((idl_member_t *)node)->is_optional be set at this point?

This is no longer relevant, after rebasing I have changed to using the IDL_ANNOTATABLE macro

dpotman · 2021-09-09T08:14:00Z

src/idl/src/annotation.c

+
+  return IDL_RETCODE_OK;
+}
+


Maybe better to move the @default implementation to a different PR (including the changes below for @default in the annotations array)? Or fix the implementation in this PR (and change the PR title)

dpotman · 2021-09-09T08:28:29Z

src/idl/src/tree.c

+}
+
+const idl_literal_t *idl_has_default(const void *node)
+{


Move to a separate PR?

With the function name idl_has_default I would expect a bool that indicates if the node has a default (which is what the function idl_is_default below does). Maybe a better name would be idl_get_default_value or something similar?

dpotman · 2021-09-09T08:33:20Z

src/idl/src/tree.c

+
+  if (lit
+   && idl_mask(lit) == (IDL_LITERAL | IDL_ANY))
+    return lit->value.bln;


Does this also work if an explicit default value 0 is applied to a member (@default(0)), this function should return true in that case?

dpotman · 2021-09-09T08:36:39Z

src/idl/tests/annotation.c

+
+  idl_delete_pstate(pstate);
+
+  const char *errstr[] = {"@optional stuct s {\n"


Typo stuct ?

You are correct.

dpotman · 2021-09-09T08:39:37Z

src/idl/tests/annotation.c

+                          "  e_2,\n"
+                          "  e_3\n"
+                          "};\n"};
+


Maybe add a test for invalid arguments, like @optional(1), @optional('true') ?

The number of test cases will be expanded a lot in the next iteration.

dpotman · 2021-09-09T08:43:11Z

src/idl/tests/annotation.c

+                           "  @optional char c_default;\n"
+                           "  @optional(true) char c_true;\n"
+                           "  @optional char c_1_default, c_2_default;\n"
+                           "};\n";


Add test some cases for other member types, e.g. aggregated types, sequences, arrays?

reicheratwork · 2021-09-09T08:53:02Z

I've reviewed these changes and found a few issues that need a fix, mainly the incomplete @default implementation and using the is_optional field instead of iterating through the annotations in idl_is_optional. And some nit-picking on the consistency of the coding style.

Thanks for taking the time to look over this PR! I will take your comments into account, though I will probably rebase the current changes onto #923 when it has been merged into master. After which some of the new features' implementation may be quite different.

k0ekk0ek · 2021-09-09T12:06:42Z

The IDL_ANNOTATABLE changes are in the master branch.

reicheratwork · 2021-09-09T12:25:01Z

The IDL_ANNOTATABLE changes are in the master branch.

Super!
I will rebase this onto the latest changes and then rework it into the IDL_ANNOTATABLE syntax

k0ekk0ek · 2021-09-13T09:45:42Z

src/idl/src/annotation.c

+  if (!value || !idl_is_literal(value)) {
+    idl_error(pstate, idl_location(annotation_appl),
+      "@default requires a value");
+    return IDL_RETCODE_SEMANTIC_ERROR;
+  }


This can be an assert. The parser checks the syntax and since there's no default, it should throw an error if there's no value specified. Probably good to add a small test for that (if there's none yet). If the test fails, we (or I) should fix the bug there.

You're correct that this should not really be checked here.
I will check whether the parser catches this, or if this requires a fix someplace else

This is correctly being checked by the parser at a previous step, and therefore I will replace this check with a simpler assert

k0ekk0ek · 2021-09-13T09:46:46Z

src/idl/src/annotation.c

+  //check whether type of literal matches type_spec of member
+  switch(idl_mask(value)) {
+    case IDL_LONG:
+      if (!idl_is_integer_type(mem_spec)) {
+        idl_error(pstate, idl_location(annotation_appl),
+          "value type of @default does not match member");
+        return IDL_RETCODE_SEMANTIC_ERROR;
+      }
+      break;
+    case IDL_DOUBLE:
+      if (!idl_is_floating_pt_type(mem_spec)) {
+        idl_error(pstate, idl_location(annotation_appl),
+          "value type of @default does not match member");
+        return IDL_RETCODE_SEMANTIC_ERROR;
+      }
+      break;
+    default:
+    {
+      idl_mask_t value_type = (idl_mask(value) ^ IDL_LITERAL);
+      if (idl_mask(mem_spec) != value_type) {
+        idl_error(pstate, idl_location(annotation_appl),
+          "value type of @default does not match member");
+        return IDL_RETCODE_SEMANTIC_ERROR;
+      }
+    }
+  }


This too, should have been done by the parser already.

I think that there might be a misunderstanding here, this is checking whether the type in the literal (the parameter field of the @default annotation) is compatible with that of the type it is being annotated to (so it should reject something like this: @default('a') long l;

k0ekk0ek · 2021-09-13T09:49:43Z

src/idl/src/annotation.c

+    if (idl_mask(const_expr) != (IDL_BOOL | IDL_LITERAL)) {
+      idl_error(pstate, idl_location(annotation_appl),
+        "@optional cannot have non-boolean values assigned");
+      return IDL_RETCODE_SEMANTIC_ERROR;
+    } else {
+      value = ((const idl_literal_t*)const_expr)->value.bln;
+    }


This is probably checked by the parser already. Like in one of my earlier comments, let's add a couple of small tests and fix the parser if it doesn't behave correctly already.

You're correct that this should not really be checked here.
I will check whether the parser catches this, or if this requires a fix someplace else

This is correctly being checked by the parser at a previous step, and therefore I will replace this check with a simpler assert

k0ekk0ek · 2021-09-13T09:51:07Z

src/idl/src/annotation.c

+        idl_error(pstate, idl_location(annotation_appl),
+          "@default_literal can only be assigned to member nodes");
+        return IDL_RETCODE_SEMANTIC_ERROR;


Indentation.

Will be fixed

k0ekk0ek · 2021-09-13T10:01:01Z

src/idl/include/idl/tree.h

@@ -378,6 +380,7 @@ struct idl_enumerator {
     mapped to a native data type capable of representing a maximally-sized
     enumeration */
  IDL_ANNOTATABLE(uint32_t) value;
+  IDL_ANNOTATABLE(bool) is_default;


In closing the enum declaration we should check if there's only one default. Plus, I'm thinking it may actually make more sense to have a default_enumerator member in the enum too/instead?

The default_enumerator could then point to the first enumerator is not explicit default is provided? (Probably works like that, but I'm not a 100% sure)

Maybe introduce IDL_DEFAULT_ENUMERATOR that equals IDL_ENUMERATOR | 1u. The (implicit) default case for unions works like that as well. Don't know if it makes sense, but it might be worth looking into that.

My attempt here was to modify the existing interface as little as possible, but your approach should result in less duplicated functionality, as the different backends will not have to implement their own checks

I will look into the default member approach

Changed to a default_enumerator pointer in the idl_enum class

k0ekk0ek · 2021-09-15T16:35:53Z

src/idl/tests/annotation.c

+    {IDL_RETCODE_ILLEGAL_EXPRESSION,  false,  "struct s { @optional(\"true\") char c; };"             },
+    {IDL_RETCODE_ILLEGAL_EXPRESSION,  false,  "struct s { @optional(123) char c; };"                  },
+    {IDL_RETCODE_ILLEGAL_EXPRESSION,  false,  "struct s { @optional(0.1) char c; };"                  },


I think it'd be nicer to cover these in more generic annotation tests(?)

My reasoning behind putting the tests for the correct parameter type in the annotation is that this is something that is unique to the @optional annotation, and in my opinion, having a whole forest of different checks of matching the parameter type with the annotation type would be less descriptive.

k0ekk0ek · 2021-09-15T16:36:30Z

src/idl/tests/annotation.c

+    {IDL_RETCODE_ILLEGAL_EXPRESSION,  false,  "struct s { @optional(123) char c; };"                  },
+    {IDL_RETCODE_ILLEGAL_EXPRESSION,  false,  "struct s { @optional(0.1) char c; };"                  },
+    {IDL_RETCODE_SEMANTIC_ERROR,      false,  "enum e {  e_0, @optional e_1};"                        },
+    {IDL_RETCODE_SYNTAX_ERROR,        false,  "union u switch (long) { case 0: @optional double d; };"}


Same for this one because we're actually testing the syntax rather than something @optional specific.

Same reasoning as previous comment.

k0ekk0ek · 2021-09-23T09:20:15Z

src/idl/tests/annotation.c

+    /* skipping this test as idl_create_annotation_appl leaks memory if idl_resolve cannot resolve the scoped name (https://github.com/eclipse-cyclonedds/cyclonedds/issues/950)
+      {"@default(e_0) enum e { e_0, e_1, e_2, e_3 };",         IDL_RETCODE_SEMANTIC_ERROR,  false, IDL_NULL,    NULL}  //setting default on enums is done through @default_literal
+    */


Better enable this and indicate the test is known to fail using the WILL_FAIL property. Then, when the fix for #950 is in, the test will pass (causing the build to fail). The advantage being that we're actively notified when a change is required.

The issue is not here that the test fails, it is that there is a memory leak that is triggered by this test, and that the builds in Azure that check for memory leaks will fail if this test is done.

I know, but CTest will mark it as a failure. Since WILL_FAIL is a CTest property, it'll get us the desired behavior right? If you prefer you may set the test to disabled. It's just that commenting it out only ensures it's never looked after again. At least, there's tons of commented out tests (or test ideas) for libidl that would've caught a bunch of things but they were never looked at or thought of again... I prefer to have some kind of notification that something is still off, at least that way no one forgets.

k0ekk0ek · 2021-09-23T09:21:56Z

src/idl/tests/annotation.c

+  }
+}
+
+CU_Test(idl_annotation, enum_default)


If you can find the time, please rename the testcase to default_literal. That way it's directly obvious in the logs/terminal what this test is meant to test (not that it's super hard to guess, but still).

- The parser was not proceeding correctly in the case of the state being IDL_SCAN_ANNOTATION_APPL_SCOPED_NAME Signed-off-by: Martijn Reicher <martijn.reicher@adlinktech.com>

- The parser was setting the node mask to IDL_ANY in stead of the correct mask for bool, char and string literals Signed-off-by: Martijn Reicher <martijn.reicher@adlinktech.com>

@optional

- Reject @optional annotations on anything other than members - Reject mixing @key and @optional fields - Add idl_is_optional function which checks for the @optional annotation for idl_members and idl_declarators on members - Add unittests for @optional field parsing Signed-off-by: Martijn Reicher <martijn.reicher@adlinktech.com>

k0ekk0ek · 2021-09-24T11:17:02Z

Just a couple of warnings in the tests, if those are solved, I'll happily merge

reicheratwork · 2021-09-24T11:23:20Z

Just a couple of warnings in the tests, if those are solved, I'll happily merge

That one escaped my notice, I will fix it immediately.

@default

- Add parsing of @default annotation fields - Reject mixing @default and @optional fields - Add idl_has_default function returns a pointer to the idl_literal_t representing the default value assigned to the node, or NULL if there is none - Add unittests for @default annotation Signed-off-by: Martijn Reicher <martijn.reicher@adlinktech.com>

- Set trough the @default_literal annotation on enumerators - Add the default_enumerator pointer in idl_enum, which is set to the enumerator with @default_literal annotation or the first if none have the annotation - Add unittests Signed-off-by: Martijn Reicher <martijn.reicher@adlinktech.com>

reicheratwork force-pushed the idl_annotation_optional branch from 58415ce to ea54440 Compare August 30, 2021 14:26

k0ekk0ek mentioned this pull request Sep 1, 2021

Keep weak references to annotatable metadata in IDL tree #923

Merged

reicheratwork force-pushed the idl_annotation_optional branch 4 times, most recently from a7b776c to 979a3ad Compare September 3, 2021 14:08

dpotman reviewed Sep 9, 2021

View reviewed changes

reicheratwork force-pushed the idl_annotation_optional branch 4 times, most recently from eaabc51 to 44ddd02 Compare September 10, 2021 15:18

reicheratwork changed the title ~~Add @optional annotation parsing~~ Add additional annotation parsing Sep 10, 2021

reicheratwork changed the title ~~Add additional annotation parsing~~ Additional annotation parsing Sep 10, 2021

k0ekk0ek reviewed Sep 13, 2021

View reviewed changes

k0ekk0ek reviewed Sep 15, 2021

View reviewed changes

reicheratwork force-pushed the idl_annotation_optional branch 4 times, most recently from 990ef0c to be6348e Compare September 17, 2021 13:29

reicheratwork force-pushed the idl_annotation_optional branch from be6348e to 909dd8c Compare September 22, 2021 08:11

k0ekk0ek reviewed Sep 23, 2021

View reviewed changes

reicheratwork mentioned this pull request Sep 24, 2021

Memory leak in idl_create_annotation_appl #950

Closed

reicheratwork added 3 commits September 24, 2021 11:31

Fix bug in tokenizing scoped names

bffd2d9

- The parser was not proceeding correctly in the case of the state being IDL_SCAN_ANNOTATION_APPL_SCOPED_NAME Signed-off-by: Martijn Reicher <martijn.reicher@adlinktech.com>

Fix bug in evaluating bool, char and string literals

2b1d67e

- The parser was setting the node mask to IDL_ANY in stead of the correct mask for bool, char and string literals Signed-off-by: Martijn Reicher <martijn.reicher@adlinktech.com>

reicheratwork force-pushed the idl_annotation_optional branch from 909dd8c to 1163532 Compare September 24, 2021 09:35

k0ekk0ek approved these changes Sep 24, 2021

View reviewed changes

reicheratwork added 2 commits September 24, 2021 13:29

reicheratwork force-pushed the idl_annotation_optional branch from 1163532 to 010289e Compare September 24, 2021 11:31

k0ekk0ek merged commit d7a7abb into eclipse-cyclonedds:master Sep 24, 2021


		idl_delete_pstate(pstate);

		const char *errstr[] = {"@optional stuct s {\n"

Additional annotation parsing #919

Additional annotation parsing #919

Conversation

reicheratwork commented Aug 30, 2021

k0ekk0ek commented Aug 30, 2021

reicheratwork commented Aug 30, 2021 • edited Loading

reicheratwork commented Aug 30, 2021

dpotman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reicheratwork commented Sep 9, 2021

k0ekk0ek commented Sep 9, 2021

reicheratwork commented Sep 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k0ekk0ek commented Sep 24, 2021

reicheratwork commented Sep 24, 2021

reicheratwork commented Aug 30, 2021 •

edited

Loading